In size(), size_of_val(self) already includes the inline [bool; 256] storage, so the extra + 256 double-counts memory. This can skew DataFusion’s memory accounting/limits for COUNT DISTINCT on small integer types.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

value:useful; category:bug; feedback: The Augment AI reviewer is correct! The seen field is an array, so it is stored in the stack space and thus is included in the calculated value by size_of_val(). It should not be counted manually too. Prevents overcalculating the size of the accumulator.

This change routes UInt8/Int8/UInt16/Int16 through new bitmap/bool-array accumulators, but there don’t appear to be unit tests covering these new distinct paths (especially state()/merge_batch() behavior). Adding coverage here would help catch regressions in distinct correctness across partial/final aggregation.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

value:good-to-have; category:bug; feedback: The Augment AI reviewer is correct! Test cases for this new code path are not added to the Pull Request and they may not behave as desired and also could regress in the future.

Iterating over the array using iter().flatten() is inefficient for primitive types because it performs an option check for every element. Since this is a performance-oriented optimization, it's better to check null_count() and use values() directly when no nulls are present.

        if arr.null_count() == 0 {
            for &value in arr.values() {
                self.seen[value as usize] = true;
            }
        } else {
            for value in arr.iter().flatten() {
                self.seen[value as usize] = true;
            }
        }

value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The flatten() operation checks each item in the array whether it is Some or None. This could be avoided for no-nulls arrays by using the constant check arr.null_count() == 0. This single check could avoid N checks for None, where N is the size of the array.

The size method is double-counting the memory used by the seen array. Since seen is a fixed-size array within the struct, size_of_val(self) already includes its size (256 bytes).

value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The seen field is an array, so it is stored in the stack space and thus is included in the calculated value by size_of_val(). It should not be counted manually too. Prevents overcalculating the size of the accumulator.

Similar to the u8 accumulator, using iter().flatten() is less efficient than direct slice access when nulls are absent. Checking null_count() allows for a faster path using values().

        if arr.null_count() == 0 {
            for &value in arr.values() {
                self.seen[value as u8 as usize] = true;
            }
        } else {
            for value in arr.iter().flatten() {
                self.seen[value as u8 as usize] = true;
            }
        }

value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The flatten() operation checks each item in the array whether it is Some or None. This could be avoided for no-nulls arrays by using the constant check arr.null_count() == 0. This single check could avoid N checks for None, where N is the size of the array.

The size method is double-counting the memory used by the seen array. size_of_val(self) already includes the 256 bytes of the fixed-size array.

value:useful; category:bug; feedback: The Gemini AI reviewer is correct! The seen field is an array, so it is stored in the stack space and thus is included in the calculated value by size_of_val(). It should not be counted manually too. Prevents overcalculating the size of the accumulator.

Performance can be improved by avoiding the overhead of Option iteration when the array has no nulls.

        if arr.null_count() == 0 {
            for &value in arr.values() {
                self.set_bit(value);
            }
        } else {
            for value in arr.iter().flatten() {
                self.set_bit(value);
            }
        }

value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The flatten() operation checks each item in the array whether it is Some or None. This could be avoided for no-nulls arrays by using the constant check arr.null_count() == 0. This single check could avoid N checks for None, where N is the size of the array.

Iterating through all 64 bits for every non-zero word is inefficient, especially for sparse bitmaps. Using trailing_zeros() to find set bits is significantly faster and more idiomatic for bitmap processing.

        for (word_idx, &word) in self.bitmap.iter().enumerate() {
            let mut w = word;
            while w != 0 {
                let bit = w.trailing_zeros();
                values.push((word_idx as u16) * 64 + bit as u16);
                w &= !(1u64 << bit);
            }
        }

value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! By using u64::trailing_zeros() the algorithm could be optimized to process only the 1s in the bitmap from back to front. This way it will iterate only the 1s which could lead to a big gain for sparse bitmaps.

Performance can be improved by avoiding the overhead of Option iteration when the array has no nulls.

        if arr.null_count() == 0 {
            for &value in arr.values() {
                self.set_bit(value);
            }
        } else {
            for value in arr.iter().flatten() {
                self.set_bit(value);
            }
        }

value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! The flatten() operation checks each item in the array whether it is Some or None. This could be avoided for no-nulls arrays by using the constant check arr.null_count() == 0. This single check could avoid N checks for None, where N is the size of the array.

Iterating through all 64 bits for every non-zero word is inefficient. Using trailing_zeros() to find set bits is significantly faster.

        for (word_idx, &word) in self.bitmap.iter().enumerate() {
            let mut w = word;
            while w != 0 {
                let bit = w.trailing_zeros();
                values.push(((word_idx as u16) * 64 + bit as u16) as i16);
                w &= !(1u64 << bit);
            }
        }

value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! By using u64::trailing_zeros() the algorithm could be optimized to process only the 1s in the bitmap from back to front. This way it will iterate only the 1s which could lead to a big gain for sparse bitmaps.

Bool array accumulators double-count memory in size()

Low Severity

The size() method for BoolArray256DistinctCountAccumulator and BoolArray256DistinctCountAccumulatorI8 returns size_of_val(self) + 256, but the seen: [bool; 256] field is stored inline in the struct — not on the heap. size_of_val(self) already includes the 256-byte array, so adding 256 double-counts it, reporting ~512 bytes instead of 256. Contrast this with the Bitmap65536 variants, which correctly add 8192 for the heap-allocated Box<[u64; 1024]> that size_of_val only accounts for as a pointer. This overestimate in memory tracking could cause premature spilling or other unnecessary memory-pressure actions.

 

^{Reviewed by Cursor Bugbot for commit b564773. Configure here.}

value:useful; category:bug; feedback: The Bugbot AI reviewer is correct! The seen field is an array, so it is stored in the stack space and thus is included in the calculated value by size_of_val(). It should not be counted manually too. Prevents overcalculating the size of the accumulator.

⚠️ Potential issue | 🟠 Major

Avoid routing all UInt16/Int16 groups to a fixed 8 KiB accumulator.

The UInt16/Int16 half of this branch now selects accumulators that eagerly allocate the full bitmap. DISTINCT still uses one Accumulator per live group here, so sparse groups go from “grow with cardinality” to “always 8 KiB”. On wide GROUP BY COUNT(DISTINCT ...) plans that is a sizable memory regression and can force early spill/OOM. Please keep the u16/i16 path lazy/adaptive instead of switching every case to the bitmap.

Verify each finding against the current code and only fix it if needed.

In `@datafusion/functions-aggregate/src/count.rs` around lines 191 - 193, The
DataType::UInt16 | DataType::Int16 match branch currently forces creation of the
full- bitmap accumulator via get_small_int_accumulator(data_type).unwrap(),
causing eager 8 KiB allocations per group; change this branch to return the
lazy/adaptive accumulator used for other small-int types (i.e., the accumulator
that grows with cardinality and only materializes a bitmap when needed) instead
of the fixed bitmap one so DISTINCT keeps one growable Accumulator per live
group; update the DataType::UInt16 | DataType::Int16 arm to use the existing
lazy/adaptive factory (or the previous get_small_int_accumulator variant that is
lazy) and ensure types/unwrap usage match the surrounding code.